Conversation
|
|
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds CI/CodeRabbit config and ESLint ignore patterns, registers several AI agent skill reference files, serves Storybook static assets, applies a one-line realtime call-site tweak, updates many docs and examples across guides/integrations/backends, and switches logo assets to a CDN; no runtime public API implementations were added. Changes
Sequence Diagram(s)(Skipped — changes are documentation-heavy plus a minor call-site tweak; no new multi-component control flow requiring visualization.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
@ginjou/core
@ginjou/nuxt
@ginjou/vue
@ginjou/with-directus
@ginjou/with-rest-api
@ginjou/with-supabase
@ginjou/with-vue-i18n
@ginjou/with-vue-router
commit: |
Deploying ginjou with
|
| Latest commit: |
dfbb6a5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://404c45b3.ginjou.pages.dev |
| Branch Preview URL: | https://docs-improve-document-conten.ginjou.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #103 +/- ##
=======================================
Coverage 55.09% 55.09%
=======================================
Files 128 128
Lines 3142 3142
Branches 771 771
=======================================
Hits 1731 1731
Misses 1252 1252
Partials 159 159 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Deploying ginjou-storybook with
|
| Latest commit: |
dfbb6a5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8c6e509e.ginjou-storybook.pages.dev |
| Branch Preview URL: | https://docs-improve-document-conten.ginjou-storybook.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/content/1.guides/5.authorization.md`:
- Around line 165-178: In authz.access, the parameter named resource is being
shadowed by the local const resource = await getResource(params.id); rename one
of them (e.g., rename the parameter to resourceParam or the local variable to
fetchedResource) and update all uses in the function (including the getResource
call and the can check) so the function uses distinct, descriptive names; ensure
authz.access still calls getCurrentUser() and returns the same can expression
comparing user.department to the correctly named resource field and checking
action in ['read','edit'].
In `@docs/content/1.guides/8.resources.md`:
- Around line 136-163: The example references an undefined resourceMap inside
buildBreadcrumbs causing runtime errors; fix by providing a source for parent
lookups: either define a resourceMap (e.g., populate a Map keyed by resource id
from resolved.value.resources) or replace resourceMap.get(...) with a
composable/helper that resolves a parent by id (create and import a function
like findResourceById) and use it inside buildBreadcrumbs; ensure you reference
useResource/resolved and update buildBreadcrumbs to use the chosen parent-lookup
mechanism so current = <resourceMap or findResourceById>(current.meta?.parent)
is valid.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
docs/content/1.guides/6.notifications.md (1)
6-8:⚠️ Potential issue | 🟡 MinorRename provider section to “Setup.”
Line 6 should use “Setup” for provider registration sections to match documentation standards.
As per coding guidelines: Use 'Setup' for provider registration sections in documentation.docs/content/1.guides/4.authentication.md (2)
6-8:⚠️ Potential issue | 🟡 MinorRename provider section to “Setup.”
Line 6 should use “Setup” for provider registration sections.
As per coding guidelines: Use 'Setup' for provider registration sections in documentation.
150-191:⚠️ Potential issue | 🟡 MinorAdd a Success Flow for
useAuthenticatedanduseGetIdentity.Both Composition sections list Data Composables and Actions but omit Success Flow; the controller doc standard requires all three elements.
Based on learnings: When documenting high-level Controllers, always explain their Composition including Data Composables, Actions, and Success Flow.docs/content/1.guides/8.resources.md (1)
6-8:⚠️ Potential issue | 🟡 MinorRename provider section to “Setup.”
Line 6 should use “Setup” for provider registration sections.
As per coding guidelines: Use 'Setup' for provider registration sections in documentation.docs/content/1.guides/7.realtime.md (1)
6-8:⚠️ Potential issue | 🟡 MinorRename provider section to “Setup.”
Line 6 should use “Setup” for provider registration sections.
As per coding guidelines: Use 'Setup' for provider registration sections in documentation.
🤖 Fix all issues with AI agents
In `@docs/content/1.guides/4.authentication.md`:
- Line 234: Update the sentence describing the checkError function to hyphenate
the compound modifier: replace the phrase "error handling patterns" with
"error-handling patterns" in the paragraph that starts "The `checkError`
function examines API errors and determines whether they indicate an
authentication failure." to ensure correct compound adjective usage.
In `@docs/content/1.guides/8.resources.md`:
- Around line 247-250: Update the comment in the call to defineResourceContext
to fix the typo: change "As mention before" to "As mentioned before" in the
inline comment near the resources argument (the defineResourceContext({
resources, // As mention before }) invocation).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
README.md (2)
35-35:⚠️ Potential issue | 🟡 MinorRemove numbered prefix from headers.
The header uses a numbered prefix "1. Installation" which violates the coding guideline. As per coding guidelines, do not use numbered headers or redundant suffixes in documentation headers.
📝 Proposed fix
-**1. Installation** +**Installation**
41-41:⚠️ Potential issue | 🟡 MinorRemove numbered prefix from headers.
The header uses a numbered prefix "2. Usage" which violates the coding guideline. As per coding guidelines, do not use numbered headers or redundant suffixes in documentation headers.
📝 Proposed fix
-**2. Usage** +**Usage**
🤖 Fix all issues with AI agents
In `@README.md`:
- Line 2: The image src currently hardcodes the PR branch in the <img> tag
(alt="Ginjou Logo") which will break after the branch is deleted; update the src
attribute to point to a stable reference (for example the main branch or a
permanent raw URL / CDN that uses the default branch or a release commit hash)
so the logo remains available after merge—locate the <img> tag in README.md and
replace the branch-specific path "docs%2Fimprove-document-content-with-ai" with
the stable branch name (e.g., "main") or a commit/tag-based URL.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/app/app.config.ts`:
- Around line 2-4: The CDN URL for the logo is pinned to a feature branch and
will break when that branch is deleted; update the assets.logo value so it
points to a stable ref (for example replace the jsdelivr tag segment
`@docs%2Fimprove-document-content-with-ai` with `@main` or a release tag/commit
SHA) or, alternatively, revert to serving the static asset from the repo (e.g.,
use the local path `/logo.png`) so the logo is not dependent on an external CDN.
🧹 Nitpick comments (1)
docs/content/1.guides/8.resources.md (1)
247-265: Consider adding import statements for clarity.The Multiple Fetchers example shows usage of
defineResourceContextanddefineFetchersContext, but doesn't include import statements. Adding these would make the example more complete and easier for developers to follow.📝 Suggested addition
+```typescript +import { defineResourceContext, defineFetchersContext } from '@ginjou/vue' + defineResourceContext({ resources, // As mentioned before })
| assets: { | ||
| logo: 'https://cdn.jsdelivr.net/gh/aa900031/ginjou@docs%2Fimprove-document-content-with-ai/assets/logo.svg', | ||
| }, |
There was a problem hiding this comment.
CDN URL references the feature branch — will break once the branch is deleted.
The jsdelivr URL is pinned to @docs%2Fimprove-document-content-with-ai. After this PR merges and the branch is deleted, jsdelivr will no longer be able to resolve the asset (cached copies eventually expire). Point the reference to @main (or a release tag / commit SHA) instead.
Also consider whether serving the logo from the repo's own public/ directory (as it was before with /logo.png) would be simpler and more resilient than depending on an external CDN for a static asset.
Proposed fix
assets: {
- logo: 'https://cdn.jsdelivr.net/gh/aa900031/ginjou@docs%2Fimprove-document-content-with-ai/assets/logo.svg',
+ logo: 'https://cdn.jsdelivr.net/gh/aa900031/ginjou@main/assets/logo.svg',
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assets: { | |
| logo: 'https://cdn.jsdelivr.net/gh/aa900031/ginjou@docs%2Fimprove-document-content-with-ai/assets/logo.svg', | |
| }, | |
| assets: { | |
| logo: 'https://cdn.jsdelivr.net/gh/aa900031/ginjou@main/assets/logo.svg', | |
| }, |
🤖 Prompt for AI Agents
In `@docs/app/app.config.ts` around lines 2 - 4, The CDN URL for the logo is
pinned to a feature branch and will break when that branch is deleted; update
the assets.logo value so it points to a stable ref (for example replace the
jsdelivr tag segment `@docs%2Fimprove-document-content-with-ai` with `@main` or
a release tag/commit SHA) or, alternatively, revert to serving the static asset
from the repo (e.g., use the local path `/logo.png`) so the logo is not
dependent on an external CDN.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/content/3.backend/supabase.md (1)
39-73:⚠️ Potential issue | 🟡 MinorBackend Setup section should demonstrate integration with Nuxt.
The Setup section currently shows Vue and Svelte examples, but according to the coding guidelines, backend documentation should include a Setup section demonstrating integration with all supported frontend frameworks (Vue and Nuxt). The Svelte example can be optional, but Nuxt integration should be included.
Based on learnings: Backend documentation should include a Setup section demonstrating integration with all supported frontend frameworks (Vue and Nuxt).
📝 Suggested addition for Nuxt example
Add a Nuxt tab to the code-group:
<script setup lang="ts"> import { defineAuthContext, defineFetchersContext } from '@ginjou/vue' import { createAuth, createFetcher } from '@ginjou/with-supabase' import { createClient } from '@supabase/supabase-js' const supabaseUrl = 'https://your-project.supabase.co' const supabaseKey = 'your-anon-key' const client = createClient(supabaseUrl, supabaseKey) defineFetchersContext({ default: createFetcher({ client }) }) defineAuthContext(createAuth({ client })) </script> <template> <NuxtLayout> <NuxtPage /> </NuxtLayout> </template>docs/content/3.backend/rest-api.md (1)
39-68:⚠️ Potential issue | 🟡 MinorBackend Setup section should demonstrate integration with Nuxt.
The Setup section currently shows Vue and Svelte examples, but according to the coding guidelines, backend documentation should include a Setup section demonstrating integration with all supported frontend frameworks (Vue and Nuxt). While Svelte can be optional, Nuxt integration should be included.
Based on learnings: Backend documentation should include a Setup section demonstrating integration with all supported frontend frameworks (Vue and Nuxt).
📝 Suggested addition for Nuxt example
Add a Nuxt tab to the code-group:
<script setup lang="ts"> import { defineFetchersContext } from '@ginjou/vue' import { createFetcher } from '@ginjou/with-rest-api' defineFetchersContext({ default: createFetcher({ url: 'https://api.example.com' }) }) </script> <template> <NuxtLayout> <NuxtPage /> </NuxtLayout> </template>docs/content/3.backend/directus.md (1)
39-73:⚠️ Potential issue | 🟡 MinorMissing Nuxt setup example; replace Svelte WIP placeholder.
The Setup section currently shows Vue and a Svelte WIP placeholder, but is missing a Nuxt example. As per coding guidelines, backend documentation should include a Setup section demonstrating integration with all supported frontend frameworks (Vue and Nuxt). The Svelte block provides no implementation and should either be completed or removed.
📋 Recommended structure
Replace the Svelte WIP block with a Nuxt example:
```vue [Vue] <script setup lang="ts"> import { authentication, createDirectus, rest } from '@directus/sdk' import { defineAuthContext, defineFetchersContext } from '@ginjou/vue' import { createAuth, createFetcher } from '@ginjou/with-directus' const client = createDirectus('https://your-directus-url.com') .with(rest()) .with(authentication()) defineFetchersContext({ default: createFetcher({ client }) }) defineAuthContext(createAuth({ client })) </script> <template> <RouterView /> </template>-```svelte [Svelte]
-
-<script>
- // ...
-</script>
+```vue [Nuxt]
+<script setup lang="ts">
+import { authentication, createDirectus, rest } from '@directus/sdk'
+import { defineAuthContext, defineFetchersContext } from '@ginjou/nuxt'
+import { createAuth, createFetcher } from '@ginjou/with-directus'
+const client = createDirectus('https://your-directus-url.com')
- .with(rest())
- .with(authentication())
+defineFetchersContext({
- default: createFetcher({ client })
+})+defineAuthContext(createAuth({ client }))
+</script>
+
+
+
Based on learnings: Backend documentation should include a Setup section demonstrating integration with all supported frontend frameworks (Vue and Nuxt).
🧹 Nitpick comments (2)
docs/content/1.guides/1.data.md (1)
102-107: Svelte WIP placeholders provide no implementation guidance.Throughout this document, Svelte code blocks contain only
<!-- WIP -->placeholders (appearing at lines 102-107, 152-157, 200-205, 249-254, 316-321, and many more). These placeholders don't provide any value to readers trying to implement Svelte integrations.Consider either:
- Implementing actual Svelte examples if the framework supports it
- Removing the WIP blocks entirely until Svelte support is ready
- Adding a note at the top stating "Svelte examples coming soon"
docs/content/1.guides/8.resources.md (1)
184-189: Svelte WIP placeholder.Similar to the data guide, this file contains Svelte WIP placeholders that provide no implementation. Consider either completing these examples or removing them until Svelte support is ready.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/content/1.guides/0.introduction.md (1)
106-134:⚠️ Potential issue | 🟠 MajorMermaid diagram should show data flow, not just call hierarchy.
The current graph is primarily a dependency/call hierarchy. Please switch to a flow/sequence diagram that shows how data moves between layers (e.g., UI → controller → composables → fetcher → backend → cache → UI).
As per coding guidelines: "Use Mermaid diagrams in documentation to show logic flow and how data moves, not just call hierarchies".docs/content/1.guides/8.resources.md (1)
6-26: 🛠️ Refactor suggestion | 🟠 MajorAdd a dedicated "Setup" section for provider registration.
The document introduces resource concepts but doesn't provide a clear "Setup" section showing how to register the resource provider. The actual registration code (
defineResourceContext) appears later in the "Multiple Fetchers" section (line 257), making it difficult for developers to understand basic setup.As per coding guidelines, use 'Setup' for provider registration sections in documentation. Add a Setup section after line 26 demonstrating basic resource provider registration for both Vue and Nuxt before diving into specific patterns.
📝 Suggested Setup section
Add after line 26:
## Setup Register your resources using the resource provider context: ::code-group --- sync: guide-example --- ```vue [Vue] <script setup lang="ts"> import { defineResourceContext } from '@ginjou/vue' const resources = [ { name: 'posts', list: '/posts', create: '/posts/new', edit: '/posts/:id/edit', show: '/posts/:id', }, ] defineResourceContext({ resources }) </script><script setup lang="ts"> // In app.vue or app/app.vue import { defineResourceContext } from '@ginjou/nuxt' const resources = [ { name: 'posts', list: '/posts', create: '/posts/new', edit: '/posts/:id/edit', show: '/posts/:id', }, ] defineResourceContext({ resources }) </script>::
</details> </blockquote></details> </blockquote></details>🤖 Fix all issues with AI agents
In `@docs/content/1.guides/0.introduction.md`: - Around line 142-151: Update the useList example to explicitly describe its Controller composition: list the Data Composables (e.g., useGetList for fetching, any query/state hooks), the Actions provided by useList (e.g., refresh, applyFilters, paginate — reference useGo/useLocation where URL-sync actions occur), and the Success Flow (how useNotify and useTranslate are invoked on successful operations and how success handlers propagate results). Mention the lower-level composables that can be combined (useGetList, useGo, useLocation) and state that useList wires those data composables, exposes action methods, and triggers notify/translate on success so readers see Data Composables, Actions, and Success Flow explicitly. In `@docs/package.json`: - Line 13: The dependency "docus": "^5.5.0" in package.json is invalid (no such version on npm); update the version spec for the "docus" dependency to a published release such as "^4.1.3" or confirm and replace it with the correct pre-release/private-registry spec, then run npm install to verify; locate the "docus" entry in package.json to make this change.🧹 Nitpick comments (2)
docs/content/1.guides/8.resources.md (2)
184-189: Complete Svelte examples for framework parity.Multiple Svelte examples are marked as "WIP" throughout the document (also at lines 278-283). While Vue examples are comprehensive, the incomplete Svelte code reduces documentation utility for Svelte users.
252-276: Consider showing usage of destructured variables.Lines 268-274 destructure
recordsanddatafromuseListbut don't demonstrate their usage in the template or elsewhere. For documentation clarity, either show how these variables are used or simplify the example to focus on the fetcher configuration aspect.
| ### Example: Building a List Page with `useList` | ||
|
|
||
| `useList` is a great example of how this works. It bundles several smaller composables to create a full solution for a list page, including: | ||
| The `useList` Controller demonstrates the composition pattern in action. It bundles several specialized composables to deliver a complete solution for list pages: | ||
|
|
||
| - Data fetching (`useGetList`) | ||
| - URL-based pagination and filtering (`useGo`, `useLocation`) | ||
| - Automatic success and error messages (`useNotify`) | ||
| - Text translation (i18n) (`useTranslate`) | ||
| - **Data fetching:** `useGetList` retrieves data from your backend | ||
| - **Pagination and filtering:** `useGo` and `useLocation` sync with the URL | ||
| - **User feedback:** `useNotify` shows success and error messages | ||
| - **Localization:** `useTranslate` handles i18n automatically | ||
|
|
||
| You can use `useList` to build your app quickly. But if you need something different, you can use the next level down (`useGetList`) and add your own logic to build a custom solution. | ||
| Use `useList` for quick development. When you need something different, step down to lower-level composables like `useGetList` and combine them with your own logic to build a custom solution. |
There was a problem hiding this comment.
Add explicit Controller composition: Data Composables, Actions, Success Flow.
This section explains capabilities but doesn’t explicitly call out the Controller’s Composition in terms of Data Composables, Actions, and Success Flow. Please add a short, explicit breakdown for useList.
Based on learnings: "When documenting high-level Controllers in documentation, always explain their Composition including Data Composables, Actions, and Success Flow".
🤖 Prompt for AI Agents
In `@docs/content/1.guides/0.introduction.md` around lines 142 - 151, Update the
useList example to explicitly describe its Controller composition: list the Data
Composables (e.g., useGetList for fetching, any query/state hooks), the Actions
provided by useList (e.g., refresh, applyFilters, paginate — reference
useGo/useLocation where URL-sync actions occur), and the Success Flow (how
useNotify and useTranslate are invoked on successful operations and how success
handlers propagate results). Mention the lower-level composables that can be
combined (useGetList, useGo, useLocation) and state that useList wires those
data composables, exposes action methods, and triggers notify/translate on
success so readers see Data Composables, Actions, and Success Flow explicitly.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/content/3.backend/supabase.md (1)
35-73:⚠️ Potential issue | 🟠 MajorMissing Nuxt integration example in Setup section.
The Setup section currently only demonstrates Vue integration. As per coding guidelines, backend documentation should include Setup examples for all supported frontend frameworks (Vue and Nuxt). The Nuxt example should show the proper root component setup with
<NuxtPage />instead of<RouterView />.As per coding guidelines: "Backend documentation should include a Setup section demonstrating integration with all supported frontend frameworks (Vue and Nuxt)"
📝 Suggested Nuxt example to add
Add a Nuxt tab to the code-group:
<script setup lang="ts"> import { defineAuthContext, defineFetchersContext } from '@ginjou/nuxt' import { createAuth, createFetcher } from '@ginjou/with-supabase' import { createClient } from '@supabase/supabase-js' const supabaseUrl = 'https://your-project.supabase.co' const supabaseKey = 'your-anon-key' const client = createClient(supabaseUrl, supabaseKey) defineFetchersContext({ default: createFetcher({ client }) }) defineAuthContext(createAuth({ client })) </script> <template> <NuxtPage /> </template>docs/content/3.backend/directus.md (1)
35-73:⚠️ Potential issue | 🟠 MajorMissing Nuxt integration example in Setup section.
The Setup section currently only demonstrates Vue integration. As per coding guidelines, backend documentation should include Setup examples for all supported frontend frameworks (Vue and Nuxt). The Nuxt example should show the proper root component setup with
<NuxtPage />instead of<RouterView />.As per coding guidelines: "Backend documentation should include a Setup section demonstrating integration with all supported frontend frameworks (Vue and Nuxt)"
📝 Suggested Nuxt example to add
Add a Nuxt tab to the code-group:
<script setup lang="ts"> import { authentication, createDirectus, rest } from '@directus/sdk' import { defineAuthContext, defineFetchersContext } from '@ginjou/nuxt' import { createAuth, createFetcher } from '@ginjou/with-directus' const client = createDirectus('https://your-directus-url.com') .with(rest()) .with(authentication()) defineFetchersContext({ default: createFetcher({ client }) }) defineAuthContext(createAuth({ client })) </script> <template> <NuxtPage /> </template>
Summary by CodeRabbit
Documentation
New Features
Chores